fix: add handle for resolveExternalUri #5624

jsjoeio · 2022-10-06T17:27:06Z

This PR fixes asExternalUri to use code-server's built-in proxy or VSCODE_PROXY_URI if it's been set.

Todos

handle path-based proxies
add e2e test for it
add e2e test for VSCODE_PROXY_URI
support VSCODE_PROXY_URI

Fixes #1936

patches/proxy-uri.diff

code-asher · 2022-10-06T18:24:37Z

Patch patches/proxy-uri.diff does not apply (enforce with -f)

Might be easiest to force-apply it then resolve the conflicts and refresh it! It could be a conflict with base-path.diff.

jsjoeio · 2022-10-06T18:53:46Z

Oops..looks like proxyEndpointTemplate adds /proxy/port for you.

code-asher

Awesome work!

patches/proxy-uri.diff

github-actions · 2022-10-06T18:56:58Z

✨ code-server dev build published to npm for PR #5624!

Last publish status: success
Commit: bfe91fc

To install in a local project, run:

npm install @coder/code-server-pr@5624

To install globally, run:

npm install -g @coder/code-server-pr@5624

codecov · 2022-10-06T18:57:58Z

Codecov Report

Merging #5624 (bfe91fc) into main (714afe0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5624   +/-   ##
=======================================
  Coverage   72.61%   72.61%           
=======================================
  Files          30       30           
  Lines        1680     1680           
  Branches      368      368           
=======================================
  Hits         1220     1220           
  Misses        397      397           
  Partials       63       63

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 714afe0...bfe91fc. Read the comment docs.

jsjoeio · 2022-10-06T19:06:09Z

Looks good now! (ignore expected, hard-coded)

code-asher

Looks great to me! Do you want an approval now to merge or are you going to add the domain-based proxy in this same PR so I should approve after that is done?

jsjoeio · 2022-10-06T20:07:09Z

are you going to add the domain-based proxy in this same PR so I should approve after that is done?

I'll work on that here then mark PR ready when all is said and done. Thanks for the early review!

jsjoeio · 2022-10-10T21:06:46Z

It appears the display language e2e test is failing. I'm going to see if I can reproduce locally.

patches/service-worker.diff

jsjoeio · 2022-10-13T17:50:37Z

test/e2e/extensions/ms-ceintl.vscode-language-pack-es-1.70.0/package.json

+	"__metadata": {
+		"id": "47e020a1-33db-4cc0-a1b4-42f97781749a",
+		"publisherDisplayName": "MS-CEINTL",
+		"publisherId": "0b0882c3-aee3-4d7c-b5f9-872f9be0a115",
+		"isPreReleaseVersion": false
+	}


I noticed VS Code added this after running the test so I decided to commit it.

jsjoeio · 2022-10-13T17:51:02Z

test/e2e/models/CodeServer.ts

@@ -327,7 +327,7 @@ export class CodeServerPage {
    const selector = "text=test extension loaded"
    this.codeServer.logger.debug("Waiting for test extension to load...")

-    await this.page.waitForSelector(selector)
+    await this.page.waitForSelector(selector, { timeout: 5000 })


gonna drop this change

jsjoeio · 2022-10-13T17:51:18Z

test/playwright.config.ts

@@ -8,6 +8,7 @@ import path from "path"
 //   yarn test:e2e --workers 1        # Run with one worker
 //   yarn test:e2e --project Chromium # Only run on Chromium
 //   yarn test:e2e --grep login       # Run tests matching "login"
+//   PWDEBUG=1 yarn test:e2e          # Run Playwright inspector


saves time from having to look this up each time 😂

100% lol same reason I added the other comments I am always forgetting the flags

jsjoeio · 2022-10-13T17:52:23Z

@code-asher ready for another review!

code-asher

Nicely done! I only have a comment about the added metadata.

test/e2e/extensions/ms-ceintl.vscode-language-pack-es-1.70.0/package.json

ericzhucode · 2023-02-15T02:44:53Z

I found there's a conflict between this feature and url displayed in open external uri dialog.
If you open an external uri for the first time then the domain of it will be resolved to cache in map data structure.
Like https://www.w3schools.com/c/c_structs.php

For the next time if you open a different uri with the same domain
Like https://github.com/coder/code-server/issues
it will be resolved as the last uri which has been cached.

The uri displayed here is not correct

Before there's no resolvers registered by default so external uri is not resolved but after this feature added, cache will take effect

Hope there's a solutaion for this. Thank you!

jsjoeio commented Oct 6, 2022

View reviewed changes

patches/proxy-uri.diff Outdated Show resolved Hide resolved

patches/proxy-uri.diff Outdated Show resolved Hide resolved

patches/proxy-uri.diff Outdated Show resolved Hide resolved

jsjoeio self-assigned this Oct 6, 2022

code-asher reviewed Oct 6, 2022

View reviewed changes

patches/proxy-uri.diff Outdated Show resolved Hide resolved

patches/proxy-uri.diff Outdated Show resolved Hide resolved

jsjoeio requested a review from code-asher October 6, 2022 18:49

code-asher reviewed Oct 6, 2022

View reviewed changes

patches/proxy-uri.diff Outdated Show resolved Hide resolved

patches/proxy-uri.diff Outdated Show resolved Hide resolved

jsjoeio temporarily deployed to npm October 6, 2022 18:56 Inactive

code-asher reviewed Oct 6, 2022

View reviewed changes

jsjoeio temporarily deployed to npm October 6, 2022 19:10 Inactive

jsjoeio force-pushed the jsjoeio/external-uri branch from 39be51a to 9681169 Compare October 7, 2022 20:24

jsjoeio temporarily deployed to npm October 7, 2022 20:26 Inactive

jsjoeio temporarily deployed to npm October 7, 2022 21:58 Inactive

jsjoeio mentioned this pull request Oct 7, 2022

asExternalUri doesn't work #1936

Closed

jsjoeio marked this pull request as ready for review October 7, 2022 22:20

jsjoeio requested a review from a team as a code owner October 7, 2022 22:20

jsjoeio requested a review from code-asher October 7, 2022 22:20

jsjoeio temporarily deployed to npm October 7, 2022 22:24 Inactive

jsjoeio temporarily deployed to npm October 10, 2022 21:08 Inactive

jsjoeio temporarily deployed to npm October 10, 2022 21:21 Inactive

jsjoeio temporarily deployed to npm October 10, 2022 21:25 Inactive

jsjoeio temporarily deployed to npm October 10, 2022 21:49 Inactive

jsjoeio temporarily deployed to npm October 10, 2022 22:07 Inactive

jsjoeio temporarily deployed to npm October 10, 2022 22:21 Inactive

jsjoeio temporarily deployed to npm October 11, 2022 01:39 Inactive

jsjoeio commented Oct 13, 2022

View reviewed changes

jsjoeio added 7 commits October 13, 2022 10:52

feat: add support for VSCODE_PROXY_URI

02a9509

chore: refresh patches

a4ed692

feat: add test for VSCODE_PROXY_URI

7a75bd0

chore: add metadata to lang extension

cfb487e

fixup!: fix part of service-worker patch

3e57d2c

fixup!: remove e2e test, update patch notes

64984bd

fixup!: refresh disable-downloads

fb7bcb8

jsjoeio force-pushed the jsjoeio/external-uri branch from d7ead89 to fb7bcb8 Compare October 13, 2022 17:52

fixup!: formatting

d763c1c

jsjoeio temporarily deployed to npm October 13, 2022 18:06 Inactive

Merge branch 'main' into jsjoeio/external-uri

36aac49

jsjoeio temporarily deployed to npm October 13, 2022 20:18 Inactive

jsjoeio temporarily deployed to npm October 13, 2022 20:30 Inactive

code-asher approved these changes Oct 13, 2022

View reviewed changes

test/e2e/extensions/ms-ceintl.vscode-language-pack-es-1.70.0/package.json Show resolved Hide resolved

Merge branch 'main' into jsjoeio/external-uri

9ce11f0

jsjoeio enabled auto-merge (squash) October 13, 2022 22:20

jsjoeio temporarily deployed to npm October 13, 2022 22:22 Inactive

Merge branch 'main' into jsjoeio/external-uri

bfe91fc

jsjoeio added the rebase when passing label Oct 14, 2022

jsjoeio temporarily deployed to npm October 14, 2022 15:59 Inactive

jsjoeio merged commit 690e0af into main Oct 14, 2022

jsjoeio deleted the jsjoeio/external-uri branch October 14, 2022 16:09

This was referenced Oct 26, 2022

Unable to sign in to TabNine behind corporate proxy/vpn codota/TabNine#526

Closed

TabNine::config page always be blank on vscode-remote codota/TabNine#305

Closed

jeromelaban mentioned this pull request Oct 27, 2022

[Bug] asExternalUri never returns gitpod-io/gitpod#14248

Closed

modaresimr mentioned this pull request Jan 3, 2023

Allow configurable viewer URL James-Yu/LaTeX-Workshop#2341

Closed

code-asher mentioned this pull request Feb 16, 2023

External URLs using wrong URL from cache #6024

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add handle for resolveExternalUri #5624

fix: add handle for resolveExternalUri #5624

jsjoeio commented Oct 6, 2022 •

edited

Loading

code-asher commented Oct 6, 2022 •

edited

Loading

jsjoeio commented Oct 6, 2022

code-asher left a comment

github-actions bot commented Oct 6, 2022 •

edited

Loading

codecov bot commented Oct 6, 2022 •

edited

Loading

jsjoeio commented Oct 6, 2022

code-asher left a comment •

edited

Loading

jsjoeio commented Oct 6, 2022

jsjoeio commented Oct 10, 2022

jsjoeio Oct 13, 2022

jsjoeio Oct 13, 2022

jsjoeio Oct 13, 2022

code-asher Oct 13, 2022

jsjoeio commented Oct 13, 2022

code-asher left a comment

ericzhucode commented Feb 15, 2023 •

edited

Loading

fix: add handle for resolveExternalUri #5624

fix: add handle for resolveExternalUri #5624

Conversation

jsjoeio commented Oct 6, 2022 • edited Loading

code-asher commented Oct 6, 2022 • edited Loading

jsjoeio commented Oct 6, 2022

code-asher left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2022 • edited Loading

codecov bot commented Oct 6, 2022 • edited Loading

Codecov Report

jsjoeio commented Oct 6, 2022

code-asher left a comment • edited Loading

Choose a reason for hiding this comment

jsjoeio commented Oct 6, 2022

jsjoeio commented Oct 10, 2022

jsjoeio Oct 13, 2022

Choose a reason for hiding this comment

jsjoeio Oct 13, 2022

Choose a reason for hiding this comment

jsjoeio Oct 13, 2022

Choose a reason for hiding this comment

code-asher Oct 13, 2022

Choose a reason for hiding this comment

jsjoeio commented Oct 13, 2022

code-asher left a comment

Choose a reason for hiding this comment

ericzhucode commented Feb 15, 2023 • edited Loading

jsjoeio commented Oct 6, 2022 •

edited

Loading

code-asher commented Oct 6, 2022 •

edited

Loading

github-actions bot commented Oct 6, 2022 •

edited

Loading

codecov bot commented Oct 6, 2022 •

edited

Loading

code-asher left a comment •

edited

Loading

ericzhucode commented Feb 15, 2023 •

edited

Loading